Skip to content

[CALCITE-7779] Optimization rule FilterAggregateTransposeRule rewrites queries to semantically non-equivalent ones - #5260

Closed
npaincomplet wants to merge 1 commit into
apache:mainfrom
npaincomplet:CALCITE-7779-filter-grouping-sets
Closed

npaincomplet wants to merge 1 commit into
apache:mainfrom
npaincomplet:CALCITE-7779-filter-grouping-sets

Conversation

@npaincomplet

@npaincomplet npaincomplet commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7779

Filter pushability

In the simple aggregate case, FilterAggregateTransposeRule pushes a predicate below an aggregate when the predicate's referenced columns all belong to the aggregate’s grouping set, because aggregate-function results are unavailable before aggregation.

In the non-simple aggregate case, FilterAggregateTransposeRule pushes a predicate below an aggregate when the predicate's referenced columns all belong to every grouping set, because a referenced column is replaced with NULL in rows produced by a grouping set that omits it, and evaluating the predicate before aggregation can change the result.

Bug description

The pushability check in the non-simple case currently compares filter-input column positions (= aggregate-output column positions) with aggregate-input column positions without remapping. The same position can identify different columns in these two row layouts, so this check does not establish that the predicate’s referenced columns belong to every grouping set. Consequently, the rule can incorrectly push a predicate and change the query result, or fail to push a predicate when doing so would be valid.

Reproducer

SELECT a, b
FROM (VALUES (0, 1, 2)) AS t(unused, a, b)
GROUP BY GROUPING SETS ((a), (a, b))
HAVING b IS NULL

After applying AGGREGATE_PROJECT_MERGE followed by FILTER_AGGREGATE_TRANSPOSE, the resulting plan is equivalent to:

SELECT a, b
FROM (VALUES (0, 1, 2)) AS t(unused, a, b)
WHERE b IS NULL
GROUP BY GROUPING SETS ((a), (a, b))

which isn't semantically equivalent to the first query.

Fix

The fix uses Mappings.target and ImmutableBitSet.permute to express each grouping set in aggregate-output positions before comparing it with the predicate's references. The existing simple-aggregate check is unchanged.

## Tests

The existing dedicated grouping-set tests use matching input and output positions. The new tests apply AGGREGATE_PROJECT_MERGE first to expose the mismatch and cover:

  • HAVING b IS NULL: keep the filter above the aggregate to preserve the subtotal row.
  • HAVING b = 2: keep the filter above the aggregate to avoid an extra subtotal row.
  • HAVING a = 1: allow pushdown because a belongs to every grouping set.

All three regression tests failed against the original rule.

… column indices without remapping for non-simple aggregates
@npaincomplet

Copy link
Copy Markdown
Contributor Author

Closing because the commit title is not the same as the Jira ticket title

@sonarqubecloud

Copy link
Copy Markdown

@mihaibudiu

Copy link
Copy Markdown
Contributor

you know, git has a command to rewrite a commit

@npaincomplet

Copy link
Copy Markdown
Contributor Author

I figured from Calcite's Developer Guide that it would be preferable to open a new PR.

In order to update the pull request, you need to commit the changes in your branch and then push the commit(s) to GitHub. You are encouraged to use regular (non-rebased) commits on top of previously existing ones. When pushing the changes to GitHub, you should refrain from using the --force parameter and its alternatives.

If that was the wrong choice, I'll keep that in mind in the future.

@mihaibudiu

Copy link
Copy Markdown
Contributor

You can do whatever you want in your fork, just don't do it in the upstream calcite repository.
And squashing requires --force.

@npaincomplet

Copy link
Copy Markdown
Contributor Author

Thanks for the tips

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants